Enable emscripten build with parallelization #1045
Conversation
|
Looking at the emscripten bug reports, I feel like the issue we got with gyroid module may be something related to the older version of emscripten that nix currently uses. Maybe building with the latest version will make things work. |
|
@elalish indeed the gyroid module issue is just due to parallelization being enabled for level set. |
pca006132
force-pushed
the
emscripten-tbb
branch
from
8f5e9bb
to
6f1189d
Compare
|
|
Mind putting this comment on the line of that change that disabled the test? I'm having trouble finding it, and seems like a good one to keep an eye on when we're getting ready to merge this. How much performance improvement do you see on our npm tests? |
flake.nix
Outdated
| manifold-tbb = manifold { }; | ||
| manifold-none = manifold { parallel = false; }; | ||
| manifold-js = manifold-emscripten { parallel = false; }; | ||
| # disable check for now |
There was a problem hiding this comment.
This one with doCheck = false disables the test for nix.
|
For timing improvement, overall it is not huge, 11805ms to 9484ms. But for individual tests, I think I saw 2x speedup or something, so this really depends on the specific test. Do note that I limited the number of pthread worker to 4. |
pca006132
force-pushed
the
emscripten-tbb
branch
from
8e3807e
to
e5960b2
Compare
|
@elalish I rewrote the nix CI to pack everything into one single action with multiple variants. |
|
This is looking great - okay if we delay merging this to after v3.0? |
|
sure |
* enable emscripten build with tbb enabled * nix: fix emscripten build, add tbb build * browser * fix python build * minor * no parallel sdf for js * disable test for emscripten-tbb * add cache for nix python build * cmake changes * fix * update * update * update * update * move python dependency handling * more changes * format * fix broken format * fix * simplify * allow emscripten with cbind * build fetched dependencies as static libs * fix static build * fix rebuild * fix extras * remove parallel.h from public API * update tests * make dependencies private * move parallel.h * missing utils.h change * fix * fix... * fix wasm tbb * add more cmake consumer tests * fix consumer * add rpath * Revert "add rpath" This reverts commit 2e8a96c. * rpath specific to macos * use override * forgot to set rpath... * remove #1045 related changes * integrate #1052 * allow version override * minor improvements * msvc changes * fix typo --------- Co-authored-by: Emmett Lalish <[email protected]>
pca006132
force-pushed
the
emscripten-tbb
branch
from
162ca3c
to
a380847
Compare
|
@elalish It seems to work reliably now, but IDK why. Maybe some linker flags were not actually applied previously, but are now applied correctly? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1045 +/- ##
=======================================
Coverage 91.72% 91.72%
=======================================
Files 30 30
Lines 5910 5910
=======================================
Hits 5421 5421
Misses 489 489 ☔ View full report in Codecov by Sentry. |
| 'var workerOptions={type:"module",workerData:"em-pthread",name:"em-pthread"};', "" | ||
| ) | ||
| data = data.replace( | ||
| "workerOptions", '{type:"module",workerData:"em-pthread",name:"em-pthread"}' |
There was a problem hiding this comment.
emscripten generating some non-static worker options that Vite cannot handle, and it can be trivially modified to become static.
Overall for the c++ test, 4x, probably more for some individual benchmark. For npm test, roughly 11s to 8s, 4x faster in some benchmark. |
Note that the parallel build is still experimental, it seems that things can be a bit finicky and I had to increase the stack size. The build is also a bit finicky, I got errors running "Gyroid module" with npm/browser, will debug that a bit later... But overall the thing works in nodejs and browser.